Enable PUT checksums #320

passaro · 2023-06-28T13:07:43Z

Description of change

Enable Crc32c checksums when uploading objects. PUT requests now have an option to enable trailing checksums.

Does this change impact existing behavior?

N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

jamesbornholt · 2023-06-28T21:42:34Z

mountpoint-s3-client/tests/put_object.rs

+#[tokio::test]
+async fn test_put_checksums() {
+    const PART_SIZE: usize = 5 * 1024 * 1024;
+    let (bucket, prefix) = get_test_bucket_and_prefix("test_put_object_abort");


Suggested change

let (bucket, prefix) = get_test_bucket_and_prefix("test_put_object_abort");

let (bucket, prefix) = get_test_bucket_and_prefix("test_put_checksums");

jamesbornholt · 2023-06-28T21:43:33Z

mountpoint-s3-client/src/object_client.rs

+impl Default for PutObjectParams {
+    fn default() -> Self {
+        Self {
+            trailing_checksums: true,
+        }
+    }
+}


I don't think this should be on by default here -- as a generic S3 client it should be opt-in. But we should turn it on by default in mountpoint_s3::Uploader.

jamesbornholt · 2023-06-28T21:43:48Z

mountpoint-s3-client/src/object_client.rs

 #[non_exhaustive]
-pub struct PutObjectParams {}
+pub struct PutObjectParams {
+    pub trailing_checksums: bool,


Needs a doc comment (specifically that it's crc32c).

Actually, maybe this should be pub checksum_config: Option<ChecksumConfig> and we re-export mountpoint_s3_crt::ChecksumConfig from this crate, so that we don't end up duplicating stuff if we add more options here? I don't feel strongly about it either way, especially since the CRT combines PUT and GET checksum config into the same struct.

I thought about that, but it's unclear to me whether we want to expose ChecksumConfig in the long run (vs something tailored to specific requests: PutObject, MPU, GetObject).
I'd rather keep it as a simple bool for now and think about a more complete API when/if we'll introduce more options.

I'd probably second having an enum/struct over boolean. Something like Option<ChecksumAlgorithm>, even if ChecksumAlgorithm will only be CRC32C today?

I'm thinking the API we're putting here matches up to this one in Java SDK: https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/services/s3/model/PutObjectRequest.Builder.html#checksumAlgorithm(software.amazon.awssdk.services.s3.model.ChecksumAlgorithm)

We don't have to re-export the CRT config struct, we could just use our own in mountpoint-s3-client if it makes more sense.

@dannycjones, we could start adding a ChecksumAlgorithm enum, but what about the location (trailer vs header)? That's part of ChecksumConfig but (I believe) only trailer is supported on multi-part uploads (which is what put_object uses). Do we want to limit the API to what a specific request allows, or just error on misconfiguration? Related: will we have a separate put_object for non-mpu? Or maybe that would also be configurable?

I don't think we need to answer any of these questions now, so my preference is for the simplest possible option, knowing we will replace it later.

I probably prefer to keep it simple for now, so I'm ok with using boolean. Maybe we open a new issue to review this configuration before we publish a new version of mountpoint-s3-client? I think that's the point where we say there will be no breaking change after this and you can safely use this new config.

jamesbornholt · 2023-06-28T21:45:50Z

mountpoint-s3-crt/src/s3/client.rs

+    }
+
+    /// Get out the inner pointer to the checksum config
+    pub fn to_inner_ptr(&self) -> *const aws_s3_checksum_config {


Suggested change

pub fn to_inner_ptr(&self) -> *const aws_s3_checksum_config {

pub(crate) fn to_inner_ptr(&self) -> *const aws_s3_checksum_config {

jamesbornholt · 2023-06-28T21:46:37Z

mountpoint-s3-crt/src/s3/client.rs

+                location: aws_s3_checksum_location::AWS_SCL_TRAILER,
+                checksum_algorithm: aws_s3_checksum_algorithm::AWS_SCA_CRC32C,
+                validate_response_checksum: false,
+                validate_checksum_algorithms: std::ptr::null_mut(),


I think?

Suggested change

location: aws_s3_checksum_location::AWS_SCL_TRAILER,

checksum_algorithm: aws_s3_checksum_algorithm::AWS_SCA_CRC32C,

validate_response_checksum: false,

validate_checksum_algorithms: std::ptr::null_mut(),

location: aws_s3_checksum_location::AWS_SCL_TRAILER,

checksum_algorithm: aws_s3_checksum_algorithm::AWS_SCA_CRC32C,

..Default::default()

jamesbornholt · 2023-06-28T21:46:49Z

mountpoint-s3-crt/src/s3/client.rs

+}
+
+impl ChecksumConfig {
+    /// Create a new [ChecksumConfig] with Crc32c traling checksums.


Suggested change

/// Create a new [ChecksumConfig] with Crc32c traling checksums.

/// Create a new [ChecksumConfig] with Crc32c trailing checksums.

Also, note that this is only for PUT. It's weird (but understandable) that the CRT combines checksums for PUT and GET into this one struct, but here we are.

Signed-off-by: Alessandro Passaro <[email protected]>

passaro temporarily deployed to PR integration tests June 28, 2023 13:07 — with GitHub Actions Inactive

passaro requested a review from monthonk June 28, 2023 13:07

passaro temporarily deployed to PR integration tests June 28, 2023 13:07 — with GitHub Actions Inactive

jamesbornholt reviewed Jun 28, 2023

View reviewed changes

passaro temporarily deployed to PR integration tests June 29, 2023 08:17 — with GitHub Actions Inactive

passaro had a problem deploying to PR integration tests June 29, 2023 08:17 — with GitHub Actions Failure

passaro had a problem deploying to PR integration tests June 29, 2023 09:25 — with GitHub Actions Failure

passaro temporarily deployed to PR integration tests June 29, 2023 09:25 — with GitHub Actions Inactive

passaro had a problem deploying to PR integration tests June 29, 2023 12:25 — with GitHub Actions Failure

passaro temporarily deployed to PR integration tests June 29, 2023 12:25 — with GitHub Actions Inactive

passaro had a problem deploying to PR integration tests June 29, 2023 14:03 — with GitHub Actions Failure

passaro temporarily deployed to PR integration tests June 29, 2023 14:03 — with GitHub Actions Inactive

Enable trailing checksums on PUT

ffaa6df

Signed-off-by: Alessandro Passaro <[email protected]>

passaro force-pushed the put-checksums branch from 58f501b to ffaa6df Compare June 29, 2023 14:15

passaro temporarily deployed to PR integration tests June 29, 2023 14:15 — with GitHub Actions Inactive

jamesbornholt approved these changes Jun 29, 2023

View reviewed changes

jamesbornholt merged commit 5c1c831 into awslabs:main Jun 29, 2023

passaro deleted the put-checksums branch June 29, 2023 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable PUT checksums #320

Enable PUT checksums #320

passaro commented Jun 28, 2023 •

edited

Loading

jamesbornholt Jun 28, 2023

jamesbornholt Jun 28, 2023

jamesbornholt Jun 28, 2023

jamesbornholt Jun 28, 2023 •

edited

Loading

passaro Jun 29, 2023

dannycjones Jun 29, 2023

passaro Jun 29, 2023

monthonk Jun 29, 2023

jamesbornholt Jun 28, 2023

jamesbornholt Jun 28, 2023

jamesbornholt Jun 28, 2023

jamesbornholt Jun 28, 2023

	let (bucket, prefix) = get_test_bucket_and_prefix("test_put_object_abort");
	let (bucket, prefix) = get_test_bucket_and_prefix("test_put_checksums");

	pub fn to_inner_ptr(&self) -> *const aws_s3_checksum_config {
	pub(crate) fn to_inner_ptr(&self) -> *const aws_s3_checksum_config {

	/// Create a new [ChecksumConfig] with Crc32c traling checksums.
	/// Create a new [ChecksumConfig] with Crc32c trailing checksums.

Enable PUT checksums #320

Enable PUT checksums #320

Conversation

passaro commented Jun 28, 2023 • edited Loading

Description of change

Does this change impact existing behavior?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesbornholt Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

passaro commented Jun 28, 2023 •

edited

Loading

jamesbornholt Jun 28, 2023 •

edited

Loading